Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feat: display a Network switch button when connected on the wrong chain #4027

Merged
merged 17 commits into from
Sep 10, 2024

Conversation

jmealy
Copy link
Contributor

@jmealy jmealy commented Aug 1, 2024

What it solves

Resolves #3743

How this PR fixes it

  • Shows a button to switch network instead of the execution/sign button, whenever the wallet is connected on the wrong chain.
  • When the wallet network has been switched to the same as the safe, the execution or sign button is displayed as before.

How to test it

This affects any place where you perform an action using your connected wallet, where you wallet network must match your safe network:

  • Executing a tx (send, batch, review spending limit tx, recover an account, etc.)
  • Signing a message
  • Changing safe notification settings
  • Activating a CF safe.
  • Speed up tx modal

Screenshots

Execute tx modal while wallet is on the wrong chain:
image

Changing notifications settings:
image

Activating CF safe:
image

Checklist

  • I've tested the branch on mobile 📱
  • I've documented how it affects the analytics (if at all) 📊
  • I've written a unit/e2e test for it (if applicable) 🧑‍💻

Copy link

github-actions bot commented Aug 1, 2024

Copy link

github-actions bot commented Aug 1, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link

github-actions bot commented Aug 1, 2024

📦 Next.js Bundle Analysis for safe-wallet-web

This analysis was generated by the Next.js Bundle Analysis action. 🤖

🎉 Global Bundle Size Decreased

Page Size (compressed)
global 947.98 KB (-6 B)
Details

The global bundle is the javascript bundle that loads alongside every page. It is in its own category because its impact is much higher - an increase to its size means that every page on your website loads slower, and a decrease means every page loads faster.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

If you want further insight into what is behind the changes, give @next/bundle-analyzer a try!

Twelve Pages Changed Size

The following pages changed size from the code in this PR compared to its base branch:

Page Size (compressed) First Load
/apps/open 53.58 KB (🟡 +249 B) 1001.56 KB
/balances 30.64 KB (🟡 +1 B) 978.62 KB
/home 60.29 KB (🟡 +1 B) 1008.28 KB
/new-safe/advanced-create 35.14 KB (🟡 +54 B) 983.13 KB
/new-safe/create 34.4 KB (🟡 +54 B) 982.39 KB
/settings/notifications 27.85 KB (🟡 +236 B) 975.83 KB
/transactions 73.84 KB (🟡 +251 B) 1021.82 KB
/transactions/history 73.8 KB (🟡 +251 B) 1021.79 KB
/transactions/messages 39.2 KB (🟡 +250 B) 987.18 KB
/transactions/msg 29.58 KB (🟡 +223 B) 977.56 KB
/transactions/queue 31.3 KB (🟡 +251 B) 979.28 KB
/transactions/tx 21.13 KB (🟡 +250 B) 969.11 KB
Details

Only the gzipped size is provided here based on an expert tip.

First Load is the size of the global bundle plus the bundle for the individual page. If a user were to show up to your website and land on a given page, the first load size represents the amount of javascript that user would need to download. If next/link is used, subsequent page loads would only need to download that page's bundle (the number in the "Size" column), since the global bundle has already been downloaded.

Any third party scripts you have added directly to your app using the <script> tag are not accounted for in this analysis

Next to the size is how much the size has increased or decreased compared with the base branch of this PR. If this percentage has increased by 20% or more, there will be a red status indicator applied, indicating that special attention should be given to this.

Copy link

github-actions bot commented Aug 1, 2024

Coverage report

St.
Category Percentage Covered / Total
🟡 Statements
78.31% (-0.27% 🔻)
11761/15018
🔴 Branches
58.97% (-0.33% 🔻)
3015/5113
🟡 Functions
65.86% (-0.2% 🔻)
1877/2850
🟡 Lines
79.7% (-0.33% 🔻)
10607/13308
Show new covered files 🐣
St.
File Statements Branches Functions Lines
🟢
... / index.tsx
100% 50% 100% 100%
Show files with reduced coverage 🔻
St.
File Statements Branches Functions Lines
🟡
... / wallets.ts
64.44% (-4.44% 🔻)
25%
70% (-10% 🔻)
69.44% (-2.78% 🔻)
🟡
... / transactions.ts
70.43% (-1.47% 🔻)
39.02%
42.86% (-2.6% 🔻)
73.47% (-1.29% 🔻)
🔴
... / sdk.ts
35.77% (-8.03% 🔻)
9.76% (-7.32% 🔻)
29.41% (-11.76% 🔻)
34.15% (-8.13% 🔻)
🟢
... / txHistorySlice.ts
85.71% (-3.57% 🔻)
57.14% 50% 91.67%
🟢
... / pendingTxsSlice.ts
80.77% (-19.23% 🔻)
100%
77.78% (-22.22% 🔻)
85% (-15% 🔻)
🔴
... / dispatch.ts
36.49% (-0.69% 🔻)
31.43% (-0.83% 🔻)
25%
35.53% (-0.68% 🔻)
🟢
... / index.tsx
82.35% (-8.82% 🔻)
66.67% (-16.67% 🔻)
50% (-12.5% 🔻)
83.87% (-9.68% 🔻)
🟢
... / useSyncSafeMessageSigner.ts
86.27% (-1% 🔻)
50% (-12.5% 🔻)
62.5%
91.11% (-0.73% 🔻)
🟢
... / index.tsx
87.5% (-0.74% 🔻)
90% (-3.33% 🔻)
33.33%
92.86% (-0.48% 🔻)
🟡
... / index.tsx
76.19% (-8.02% 🔻)
50% (+33.33% 🔼)
50%
77.78% (-9.72% 🔻)
🔴
... / manifest.ts
39.53% (-2.33% 🔻)
4.17%
42.86% (+14.29% 🔼)
42.11% (-5.26% 🔻)
🔴
... / useTxHistory.ts
43.33% (-53.33% 🔻)
0% (-66.67% 🔻)
0% (-100% 🔻)
42.86% (-53.57% 🔻)
🟢
... / index.tsx
85.88% (-0.32% 🔻)
70.59% 100%
86.59% (-0.32% 🔻)
🟢
... / useDeployGasLimit.ts
97.5% (-0.06% 🔻)
90.91% 100% 100%
🔴
... / RecoverAccountFlowReview.tsx
46.38% (-0.1% 🔻)
0% 0%
46.38% (-0.1% 🔻)

Test suite run success

1465 tests passing in 201 suites.

Report generated by 🧪jest coverage report action from 041ec5f

Copy link

github-actions bot commented Aug 6, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@jmealy jmealy marked this pull request as ready for review August 6, 2024 15:08
@TanyaEfremova
Copy link
Contributor

Looks good, my suggestions are:

  • Show a network logo instead of the color
  • Align text styles: both the label and the network name should use Body 1

Question: once the network is switched, will the signature request pop up automatically?

@jmealy
Copy link
Contributor Author

jmealy commented Aug 7, 2024

Looks good, my suggestions are:

  • Show a network logo instead of the color
  • Align text styles: both the label and the network name should use Body 1

Question: once the network is switched, will the signature request pop up automatically?

After the network is switched, the normal execute button will show so it would be a separate action.

Comment on lines 61 to 62
console.log(container)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
console.log(container)

@@ -45,17 +45,17 @@ import { useMemo, useState } from 'react'
export const NetworkFee = ({
totalFee,
chain,
willRelay,
strikethrough,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

strikethrough refers to how it looks instead of its function. It should be something like isWaived instead.

Copy link

github-actions bot commented Aug 8, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@jmealy jmealy requested a review from katspaugh August 9, 2024 07:40
variant="contained"
disableElevation
>
{isDisabled ? 'Waiting on confirmation in wallet...' : 'Confirm'}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on confirmation in wallet... – that's a long piece of text for a button, how does it look in this state?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changed to a loading spinner like we have it on the execute/sign buttons

Copy link
Member

@katspaugh katspaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code looks good.

@TanyaEfremova could you do a final design and usability review?

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

Copy link
Contributor

@TanyaEfremova TanyaEfremova left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like how the button looks like, however, it seems that the button and the message are too far from each other. After testing it out, it looks like they are disconnected from each other. I wonder if it would make sense to combine the alert and the button together following the pattern that we already have? (e.g. see creating a wallet screen)
Screenshot 2024-08-20 at 18 16 01

@jmealy
Copy link
Contributor Author

jmealy commented Aug 28, 2024

I like how the button looks like, however, it seems that the button and the message are too far from each other. After testing it out, it looks like they are disconnected from each other. I wonder if it would make sense to combine the alert and the button together following the pattern that we already have? (e.g. see creating a wallet screen)

Yeah sure, makes sense! So the warning would have the switch network button and then the execute/sign button would just be disabled, is that right?

Copy link

github-actions bot commented Aug 28, 2024

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@TanyaEfremova
Copy link
Contributor

Looks good, @jmealy!

@francovenica
Copy link
Contributor

francovenica commented Sep 6, 2024

The button to go to the safe creation page doesn't work. This was an issue reported in the #4020 a few weeks ago. It had to do with the list of safe not opening.
I'm sure this issue then is not from this ticket, still make sure it doesn't reach dev
image

@francovenica
Copy link
Contributor

The rest looks fine.
I'll pass the ticket to done, just make sure to check the button I mentioned in the previous comment

Copy link

ESLint Summary View Full Report

Annotations are provided inline on the Files Changed tab. You can also see all annotations that were generated on the annotations page.

Type Occurrences Fixable
Errors 0 0
Warnings 0 0
Ignored 0 N/A
  • Result: ✅ success
  • Annotations: 0 total

Report generated by eslint-plus-action

@jmealy jmealy merged commit 3cb5f57 into dev Sep 10, 2024
16 checks passed
@jmealy jmealy deleted the network-switch-button branch September 10, 2024 09:55
@github-actions github-actions bot locked and limited conversation to collaborators Sep 10, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Display a Switch Network button if wallet is connected to a different chain
4 participants